Skip to content

Conversation

@leonardt
Copy link
Collaborator

@leonardt leonardt commented Dec 18, 2019

Depends on leonardt/verilogAST-cpp#29

There's still more testing to do, but figured this would be a good time to get some initial feedback on the general design and to solicit help on weird edge cases to consider/test.

@leonardt leonardt requested a review from rsetaluri December 18, 2019 01:20
@leonardt
Copy link
Collaborator Author

As an example, this json file:

{"top":"global.test_two_ops",
 "namespaces":{
   "global":{
     "modules":{
       "Add8_cin":{
         "type":["Record",[
           ["I0",["Array",8,"BitIn"]],
           ["I1",["Array",8,"BitIn"]],
           ["O",["Array",8,"Bit"]],
           ["CIN","BitIn"]
         ]],
         "instances":{
           "bit_const_0_None":{
             "modref":"corebit.const",
             "modargs":{"value":["Bool",false]}
           },
           "inst0":{
             "genref":"coreir.add",
             "genargs":{"width":["Int",8]}
           },
           "inst1":{
             "genref":"coreir.add",
             "genargs":{"width":["Int",8]}
           }
         },
         "connections":[
           ["inst1.in0.1","bit_const_0_None.out"],
           ["inst1.in0.2","bit_const_0_None.out"],
           ["inst1.in0.3","bit_const_0_None.out"],
           ["inst1.in0.4","bit_const_0_None.out"],
           ["inst1.in0.5","bit_const_0_None.out"],
           ["inst1.in0.6","bit_const_0_None.out"],
           ["inst1.in0.7","bit_const_0_None.out"],
           ["inst1.out","inst0.in0"],
           ["self.I1","inst0.in1"],
           ["self.O","inst0.out"],
           ["self.CIN","inst1.in0.0"],
           ["self.I0","inst1.in1"]
         ]
       },
       "Sub8":{
         "type":["Record",[
           ["I0",["Array",8,"BitIn"]],
           ["I1",["Array",8,"BitIn"]],
           ["O",["Array",8,"Bit"]]
         ]],
         "instances":{
           "bit_const_1_None":{
             "modref":"corebit.const",
             "modargs":{"value":["Bool",true]}
           },
           "inst0":{
             "genref":"coreir.not",
             "genargs":{"width":["Int",8]}
           },
           "inst1":{
             "modref":"global.Add8_cin"
           }
         },
         "connections":[
           ["inst1.CIN","bit_const_1_None.out"],
           ["self.I1","inst0.in"],
           ["inst1.I1","inst0.out"],
           ["self.I0","inst1.I0"],
           ["self.O","inst1.O"]
         ]
       },
       "test_two_ops":{
         "type":["Record",[
           ["I0",["Array",8,"BitIn"]],
           ["I1",["Array",8,"BitIn"]],
           ["O",["Array",8,"Bit"]]
         ]],
         "instances":{
           "inst0":{
             "genref":"coreir.add",
             "genargs":{"width":["Int",8]}
           },
           "inst1":{
             "modref":"global.Sub8"
           }
         },
         "connections":[
           ["self.I0","inst0.in0"],
           ["self.I1","inst0.in1"],
           ["inst1.I0","inst0.out"],
           ["self.I0","inst1.I1"],
           ["self.O","inst1.O"]
         ]
       }
     }
   }
 }
 } 

now produces this verilog

module Add8_cin (input CIN, input [7:0] I0, input [7:0] I1, output [7:0] O);
 assign O = (({0,0,0,0,0,0,0,CIN}) + I0) + I1;
 endmodule

 module Sub8 (input [7:0] I0, input [7:0] I1, output [7:0] O);
 wire [7:0] inst1_O;
 Add8_cin inst1(.CIN(1), .I0(I0), .I1(~ I1), .O(inst1_O));
 assign O = inst1_O;
 endmodule

 module test_two_ops (input [7:0] I0, input [7:0] I1, output [7:0] O);
 wire [7:0] inst1_O;
 Sub8 inst1(.I0(I0 + I1), .I1(I0), .O(inst1_O));
 assign O = inst1_O;
 endmodule

@leonardt
Copy link
Collaborator Author

The logic works by providing an AST corresponding to the expression to implement a subset of the primitives. When the backend encounters these primitives, it inserts the expression rather than the standard module instantiation. Then, it runs the assign inliner pass to to remove wires where possible.

Note that this could cause wires to be removed that were explicit in the original source (e.g. a temporary python value). To handle this, perhaps we could give the user the ability to insert a wire instance or some other metadata to mark a wire as "not inlineable".

See the verilogAST PR for more info on the inlining logic. (TODO is add some unit tests for some code that had to be added when supporting this PR).

@leonardt
Copy link
Collaborator Author

"Reverse inlining" of outputs has been added, the output now looks like

module Add8_cin (input CIN, input [7:0] I0, input [7:0] I1, output [7:0] O);
assign O = (({0,0,0,0,0,0,0,CIN}) + I0) + I1;
endmodule

module Sub8 (input [7:0] I0, input [7:0] I1, output [7:0] O);
Add8_cin inst1(.CIN(1), .I0(I0), .I1(~ I1), .O(O));
endmodule

module test_two_ops (input [7:0] I0, input [7:0] I1, output [7:0] O);
Sub8 inst1(.I0(I0 + I1), .I1(I0), .O(O));
endmodule

@leonardt
Copy link
Collaborator Author

leonardt commented Dec 19, 2019

  • TODO: The concat 0s should be 1'b0 (done)

@rdaly525
Copy link
Owner

Note that this could cause wires to be removed that were explicit in the original source (e.g. a temporary python value). To handle this, perhaps we could give the user the ability to insert a wire instance or some other metadata to mark a wire as "not inlineable".

This could possibly be done whenever there are 'coreir.wire' instances in the circuit.

#include "verilogAST.hpp"
namespace vAST = verilogAST;
namespace CoreIR {
class Primitive {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this VerilogPrimitive?

@rdaly525
Copy link
Owner

This looks like great progress towards better verilog generation

@leonardt leonardt requested a review from rdaly525 January 6, 2020 18:41
Copy link
Owner

@rdaly525 rdaly525 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little hazy on the details of the transformations, but the tests look good. Also really great that the Coreir/bit primitive verilog is now stored as the AST instead of the strings.

@leonardt
Copy link
Collaborator Author

leonardt commented Jan 6, 2020

This hides the inlining logic behind the --inline flag so it shouldn't break existing code, but I'm going to verify that the magma/mantle test suite still pass before merging. I'll also see if I can update those tests to use this flag to test that it works on a broader set of a cases.

@leonardt
Copy link
Collaborator Author

leonardt commented Jan 7, 2020

Depends on leonardt/verilogAST-cpp#32

@leonardt
Copy link
Collaborator Author

leonardt commented Jan 7, 2020

Ok, based on https://github.com/phanrahan/mantle/pull/166/files it doesn't seem to break the original flow (cause a segfault or something like that) and it doesn't seem to crash on the new flow (m.compile(..., inline=True)), I think this should be ready to ship out and test with user code

@leonardt leonardt merged commit 5279e70 into master Jan 11, 2020
@leonardt leonardt deleted the inline-operators branch January 11, 2020 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants